-
Notifications
You must be signed in to change notification settings - Fork 201
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(deps): bump mongosh, driver to latest COMPASS-9056 #6774
Conversation
This includes adjusting the CSFLE/QE collection tracker for the Node.js driver changes related to `$lookup` support, where filters for listCollections can now list multiple collections in an `$in` clause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks reasonable to me, but obviously would be a good idea to get a review from someone more knowledgeable.
`[Compass] Unexpected listCollections request on '${dbName}' with name: '${ | ||
filter?.name as string | ||
}'` | ||
`[Compass] Unexpected listCollections request on '${dbName}' with filter: ${EJSON.stringify( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Should we include the shape of the filter that we expect? I find it a much nicer DX when I get an error that not only tells me that I did something wrong, but also helps me do the right thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but honestly I don't think that would be worth it, if this actually gets thrown at some point, that means we're going to have to do some digging into the root cause anyway
@@ -437,53 +446,70 @@ export class CSFLECollectionTrackerImpl implements CSFLECollectionTracker { | |||
filter: Document, | |||
collectionInfos: CollectionInfo[] | |||
): Error | undefined { | |||
if (typeof filter?.name !== 'string' || collectionInfos.length > 1) { | |||
// filter is either { name: string } or { name: { $in: string[] } } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be documented as a type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but the point here is that the input isn't of a known type ... we could split this out into a separate validation function if you prefer, that takes a generic unknown
or Document
input and returns this type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I don't have strong feelings about it; leaving it as is is fine also 👍
This includes adjusting the CSFLE/QE collection tracker for the Node.js driver changes related to
$lookup
support, where filters for listCollections can now list multiple collections in an$in
clause.Description
Checklist
Motivation and Context
Open Questions
Dependents
Types of changes